rustbuild: Fail the build if we build Cargo twice #49053
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
r? @kennytm or @Mark-Simulacrum Note that this currently fails to build because Cargo's getting compiled twice on CI, but I hope to fix that with rust-lang/cargo#5188 by the time this is approved and otherwise ready to land. |
kennytm
added
S-waiting-on-author
|
With this PR the build currently fails with: |
| @@ -253,6 +253,10 @@ pub struct Build { | |||
| ci_env: CiEnv, | |||
| delayed_failures: RefCell<Vec<String>>, | |||
| prerelease_version: Cell<Option<u32>>, | |||
| tool_artifacts: RefCell<HashMap< | |||
| Interned<String>, | |||
| HashMap<String, (&'static str, PathBuf, Vec<String>)> | |||
There was a problem hiding this comment.
Please either document these fields here or make this a struct instead of a tuple.
|
So it's not 100% clear to me why this is only done in ToolBuild. I'd imagine we should avoid recompiling crates across rust because of features, though perhaps we'll have to limit that to out-of-tree crates. Does that expansion seem reasonable? |
|
A good point! I think, though, that we're covered in the other locations for now. The other three locations, std/test/rustc, all are self-protecting effectively against this sort of error. For both std/test we don't build more than one project in those directories so we wouldn't be able to detect duplicates (and not sure what that would mean in that context). For rustc, though, we build both So in that sense I think that all we need to check here is the tools themselves, everything else should already be taken care of. |
|
Sounds good. Pending a cargo update r=me I think... |
|
PR with Cargo update: #48986 (comment) |
alexcrichton
force-pushed
the
fail-if-build-cargo-twice
branch
from
f1a677a to
7acaf46
Compare
|
I think this PR is now green but it breaks the RLS (see rust-lang/rls#767). I'll also hold off on merging at least until @matklad's regression is fixed |
|
☔ The latest upstream changes (presumably #48097) made this pull request unmergeable. Please resolve the merge conflicts. |
alexcrichton
force-pushed
the
fail-if-build-cargo-twice
branch
from
7acaf46 to
90f1568
Compare
|
@bors: r=Mark-Simulacrum |
|
📌 Commit 90f1568 has been approved by |
bors
added
S-waiting-on-bors
|
@bors r- Needs to fix error-index-generator vs rustbook. The error comes from compiling error-index-generator at stage0 and rustbook at stage2. |
bors
added
S-waiting-on-author
|
☔ The latest upstream changes (presumably #48986) made this pull request unmergeable. Please resolve the merge conflicts. |
alexcrichton
force-pushed
the
fail-if-build-cargo-twice
branch
from
90f1568 to
18e7686
Compare
bors
added a commit
to rust-lang/cargo
that referenced
this pull request
Mar 26, 2018
More effort to only compile Cargo once Hopefully one final change necessary for rust-lang/rust#49053
alexcrichton
force-pushed
the
fail-if-build-cargo-twice
branch
from
661a2bc to
e9a02ee
Compare
|
@bors: r=Mark-Simulacrum |
|
📌 Commit e9a02ee has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit e9a02ee6fbb59a4039644cb91c350195607b8459 with merge 5d5f9f67aa036160dfa6957ae16ee5ac1ebb23ca... |
|
💔 Test failed - status-travis |
bors
added
S-waiting-on-review
kennytm
added
S-waiting-on-author
|
Updating cargo broke RLS again? 🤔 |
|
☔ The latest upstream changes (presumably #49255) made this pull request unmergeable. Please resolve the merge conflicts. |
This commit updates the `ToolBuild` step to stream Cargo's JSON messages, parse them, and record all libraries built. If we build anything twice (aka Cargo) it'll most likely happen due to dependencies being recompiled which is caught by this check.
alexcrichton
force-pushed
the
fail-if-build-cargo-twice
branch
from
e9a02ee to
faebcc1
Compare
|
@bors: r=Mark-Simulacrum |
|
📌 Commit faebcc1 has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Mar 26, 2018
…-Simulacrum rustbuild: Fail the build if we build Cargo twice This commit updates the `ToolBuild` step to stream Cargo's JSON messages, parse them, and record all libraries built. If we build anything twice (aka Cargo) it'll most likely happen due to dependencies being recompiled which is caught by this check.
|
☀️ Test successful - status-appveyor, status-travis |
This was referenced Mar 27, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit updates the
ToolBuildstep to stream Cargo's JSON messages, parsethem, and record all libraries built. If we build anything twice (aka Cargo)
it'll most likely happen due to dependencies being recompiled which is caught by
this check.